core: voluntary exit #648

leolara · 2022-06-01T11:30:14Z

Adds support for voluntary exits via a new DutyExit.

category: feature
ticket: #553

codecov · 2022-06-01T11:31:30Z

Codecov Report

Merging #648 (e400958) into main (87c2054) will decrease coverage by 0.10%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
- Coverage   55.23%   55.13%   -0.11%     
==========================================
  Files          98       98              
  Lines        9477     9652     +175     
==========================================
+ Hits         5235     5322      +87     
- Misses       3492     3560      +68     
- Partials      750      770      +20

Impacted Files	Coverage Δ
testutil/beaconmock/beaconmock.go	`67.53% <0.00%> (-1.81%)`	⬇️
core/bcast/bcast.go	`31.66% <25.00%> (-2.43%)`	⬇️
core/validatorapi/validatorapi.go	`54.90% <50.87%> (+2.40%)`	⬆️
core/sigagg/sigagg.go	`58.67% <57.14%> (-0.21%)`	⬇️
core/encode.go	`55.88% <63.63%> (+1.49%)`	⬆️
core/validatorapi/router.go	`65.08% <75.00%> (+0.24%)`	⬆️
core/types.go	`43.24% <100.00%> (+0.77%)`	⬆️
core/scheduler/scheduler.go	`74.26% <0.00%> (-0.60%)`	⬇️
cmd/run.go	`100.00% <0.00%> (ø)`
p2p/config.go	`62.50% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87c2054...e400958. Read the comment docs.

corverroos · 2022-06-01T11:34:06Z

core/types.go

+	DutyProposer      DutyType = 1
+	DutyAttester      DutyType = 2
+	DutyRandao        DutyType = 3
+	DutyVoluntaryExit DutyType = 4


I would propose we rename this to DutyExit, since there is only this type of exit duty, there are no non-voluntary or any other type of exit duty. This also aligns with the other duties above, which is just DutyProposer and DutyRandao instead of more verbose redundant versions.

I don't fell comfortable with this, there will be other Exits in the code, could we ask for feedback to @OisinKyne @dB2510 @xenowits ?

corverroos · 2022-06-01T11:34:53Z

core/bcast/bcast.go

+		ve := new(eth2p0.SignedVoluntaryExit)
+		err := json.Unmarshal(aggData.Data, ve)
+		if err != nil {
+			return errors.Wrap(err, "json decoding voluntary exit")
+		}


Please follow above pattern of extracting this to core.DecodeExitAggSignedData

corverroos · 2022-06-01T11:35:41Z

core/bcast/bcast.go

+		if err == nil {
+			log.Info(ctx, "Voluntary exit successfully submitted to beacon node",
+				z.U64("epoch", uint64(ve.Message.Epoch)),
+				z.U64("validatorIndex", uint64(ve.Message.ValidatorIndex)),


please use snake case for json logging fields (see guidelines)

I would also add the pubkey here: z.Any("pubkey", pubkey)

corverroos · 2022-06-01T11:38:04Z

core/bcast/bcast_test.go

+	ve := &eth2p0.SignedVoluntaryExit{
+		Message: &eth2p0.VoluntaryExit{
+			Epoch:          10,
+			ValidatorIndex: 10,
+		},
+		Signature: testutil.RandomEth2Signature(),
+	}


you could extract this as testutil.RandomSignedVoluntaryExit

i think it is too small to extract plus it would be used only here

corverroos · 2022-06-01T11:39:02Z

core/bcast/bcast_test.go

+	aggDataData, err := json.Marshal(ve)
+	require.NoError(t, err)
+
+	aggData := core.AggSignedData{
+		Data:      aggDataData,
+		Signature: core.SigFromETH2(ve.Signature),
+	}


extract this to core.EncodeExitAggSignedData

corverroos · 2022-06-01T11:50:23Z

core/validatorapi/validatorapi.go

+		return errors.New("cannot find validator", z.U64("ValidatorIndex", uint64(ve.Message.ValidatorIndex)))
+	}
+
+	pubKeyBytes, err := vs[ve.Message.ValidatorIndex].PubKey(ctx)


this will panic if the map contains a different index.

can rename variable to eth2Pubkey

Done both things

corverroos · 2022-06-01T11:53:15Z

core/validatorapi/validatorapi.go

+	data, err := json.Marshal(ve)
+	if err != nil {
+		return nil
+	}
+
+	parSigDate := core.ParSignedData{
+		Data:      data,
+		Signature: core.SigFromETH2(ve.Signature),
+		ShareIdx:  c.shareIdx,
+	}
+
+	parsigSet := core.ParSignedDataSet{
+		pubKey: parSigDate,
+	}


can use core.Encode...

corverroos · 2022-06-01T11:54:21Z

core/validatorapi/validatorapi.go

+	duty := core.Duty{
+		Type: core.DutyVoluntaryExit,
+		Slot: math.MaxInt64,
+	}


I would use slot 0, since a zero value in golang often means "unset" and slot 0 isn't a valid eth2 slot.

or actually, we should probably convert the epoch into a slot (1st slot in the epoch) and use that. Just incase multiple exits are requested with different epochs.

ok, I do it but I will put a reference that it was your idea

corverroos · 2022-06-01T11:55:21Z

eth2util/signing/signing.go

@@ -33,7 +33,7 @@ const (
 	DomainBeaconAttester DomainName = "DOMAIN_BEACON_ATTESTER"
 	DomainRandao         DomainName = "DOMAIN_RANDAO"
 	// DomainDeposit        	         DomainName = "DOMAIN_DEPOSIT"
-	// DomainVoluntaryExit               DomainName = "DOMAIN_VOLUNTARY_EXIT"
+	DomainVoluntaryExit DomainName = "DOMAIN_VOLUNTARY_EXIT"


can move this up to just below DomainRandao

corverroos · 2022-06-01T11:56:06Z

core/validatorapi/validatorapi.go

+	if err != nil {
+		return err
+	}
+	if len(vs) != 1 {


can do else if since it is part of the error handling/validator of the above call.

I have changed this code, so this `if len' is not anymore

corverroos · 2022-06-01T11:58:31Z

Not sure the description of the PR is correct. This doesn't have anything to do with offline validators? This is just adding support for voluntary exit data...?

corverroos · 2022-06-02T07:17:48Z

core/bcast/bcast.go

@@ -72,6 +77,7 @@ func (b Broadcaster) Broadcast(ctx context.Context, duty core.Duty,
 				z.U64("slot", uint64(att.Data.Slot)),
 				z.U64("target_epoch", uint64(att.Data.Target.Epoch)),
 				z.Hex("agg_bits", att.AggregationBits.Bytes()),
+				z.Any("pubkey", pubkey.String()),


can remove .String() since z.Any does that automatically

…n go components

corverroos · 2022-06-02T07:21:00Z

core/validatorapi/validatorapi.go

+		// By instructions of @corverroos I am putting an epoch into a field called Slot
+		// https://github.com/ObolNetwork/charon/pull/648#discussion_r886724647


This comment doesn't seem very professional

I am happy to remove it, and you add the code in a commit under your name.

If this is a bad decision, that you or the company should be ashamed of, please let me know why we are taking it, when there are better alternatives.

so, do you want me to remove it?

corverroos · 2022-06-02T07:22:10Z

core/validatorapi/validatorapi.go

+		Type: core.DutyVoluntaryExit,
+		// By instructions of @corverroos I am putting an epoch into a field called Slot
+		// https://github.com/ObolNetwork/charon/pull/648#discussion_r886724647
+		Slot: int64(ve.Message.Epoch),


I meant converting the epoch into the first slot of the epoch. not just using the epoch itself.

that is not what you said, do you want to change it again?

yes please.

Note I said:

or actually, we should probably convert the epoch into a slot (1st slot in the epoch) and use that. Just incase multiple exits are requested with different epochs.

ok so I revert it

@corverroos you are editing comments to change what you said

corverroos · 2022-06-02T07:23:10Z

testutil/random.go

+	}
+}
+
+func RandomSignedVoluntaryExit(t *testing.T) *eth2p0.SignedVoluntaryExit {


t not required since not used

corverroos · 2022-06-02T07:23:18Z

testutil/random.go

@@ -376,3 +376,24 @@ func RandomUnsignedDataSet(t *testing.T) core.UnsignedDataSet {
 		RandomCorePubKey(t): unsigned,
 	}
 }
+
+func RandomVoluntaryExit(t *testing.T) *eth2p0.VoluntaryExit {


t not required since not used

it isn't used, so you do not need to include it, some random things require it, others don't.

corverroos · 2022-06-02T07:23:57Z

testutil/random.go

+	t.Helper()
+
+	return &eth2p0.SignedVoluntaryExit{
+		Message: &eth2p0.VoluntaryExit{


can replace with RandomVoluntaryExit()

corverroos · 2022-06-02T07:51:22Z

core/encode.go

@@ -102,6 +102,22 @@ func EncodeAttestationParSignedData(att *eth2p0.Attestation, shareIdx int) (ParS
 	}, nil
 }

+// EncodeVoluntaryExitParSignedData encodes to json to pass between Go components losing typing,
+// returns a ParSignedData that contains json.
+// WARNING: using this method makes you lose Golang type safety features.


please remove unprofessional comments

What is unprofessional?

Putting personal style disagreements into the code as comments

This is not a personal style, this is a statement of fact, that other contributors should be aware of.

If you think this decision is shameful, we should decide about why we are doing it this way, when there are easier ways.

You can find in Open Source software this type of comments very often, that using a method is dangerous for example.

https://github.com/ethereum/go-ethereum/blob/d8ff53dfb8a516f47db37dbc7fd7ad18a1e8a125/accounts/scwallet/README.md?plain=1#L11

https://github.com/ethereum/go-ethereum/blob/8a134014b4b370b4a3632e32a2fc8e84ee2b6947/crypto/bls12381/arithmetic_fallback.go#L26-L30

https://github.com/ethereum/go-ethereum/blob/d8ff53dfb8a516f47db37dbc7fd7ad18a1e8a125/metrics/meter.go#L104

https://github.com/ethereum/go-ethereum/blob/41671d449f5b3a19119eef070151ec72fdbb31f8/build/nsis.envvarupdate.nsh#L5-L6

https://github.com/ethereum/go-ethereum/blob/1941c5e6c99689be2e27b3e537bd591688b717de/core/rawdb/chain_freezer.go#L46-L48

corverroos

I disagree with putting personal style disagreements into the code as comments.

leolara · 2022-06-02T09:37:31Z

| Not sure the description of the PR is correct. This doesn't have anything to do with offline validators? This is just adding support for voluntary exit data...?

The PR description is from the ticket from @OisinKyne

if you have a better description please change it.

leolara · 2022-06-02T09:39:00Z

@corverroos if you want to put something else in the Slot of the duty, please push it as a commit to this branch, I am confused about what you want there

leolara · 2022-06-02T12:45:04Z

@corverroos I have remove the WARNING comment and changed VoluntaryExit to Exit.

The epoch thing I don't understand please add a commit to the branch before merging.

Cheers!

core: voluntary exit

18a44ae

corverroos reviewed Jun 1, 2022

View reviewed changes

leolara added 7 commits June 2, 2022 08:12

Fix log key style and add pubkey to logs

45855db

Avoid error in retrieving validator

6fc95da

Change variable name

bc2c1d1

Move constant DomainVoluntaryExit

9ac9a3e

Put epoch into slot

a6ef600

Extract random voluntary exit for tests

ee1b6df

clean up

6f5771f

corverroos reviewed Jun 2, 2022

View reviewed changes

Add EncodeVoluntaryExitParSignedData to encode in json to pass betwee…

1803159

…n go components

corverroos reviewed Jun 2, 2022

View reviewed changes

leolara added 3 commits June 2, 2022 10:55

Add DecodeSignedVoluntaryExitParSignedData

3e7f17d

Add EncodeVoluntaryExitParSignedData

cec382c

Remove .String()

04f9af5

corverroos suggested changes Jun 2, 2022

View reviewed changes

leolara added 2 commits June 2, 2022 11:33

Refactor RandomSignedVoluntaryExit

e8b3a5c

Revert setting epoch in slot

2b291fe

leolara added 2 commits June 2, 2022 14:37

Remove warning

04333a0

Change VoluntaryExit -> Exit

a3ea88a

corverroos added 2 commits June 3, 2022 15:21

cleanup

6591fb3

cleanup

a6ed288

corverroos approved these changes Jun 3, 2022

View reviewed changes

cleanup

e400958

corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jun 3, 2022

obol-bulldozer bot merged commit 94e616b into main Jun 3, 2022

obol-bulldozer bot deleted the leolara/voluntary-exit branch June 3, 2022 13:32

		// By instructions of @corverroos I am putting an epoch into a field called Slot
		// https://github.com/ObolNetwork/charon/pull/648#discussion_r886724647

core: voluntary exit #648

core: voluntary exit #648

Conversation

leolara commented Jun 1, 2022 • edited by corverroos Loading

codecov bot commented Jun 1, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos left a comment

Choose a reason for hiding this comment

leolara commented Jun 2, 2022

leolara commented Jun 2, 2022

leolara commented Jun 2, 2022

leolara commented Jun 1, 2022 •

edited by corverroos

Loading

codecov bot commented Jun 1, 2022 •

edited

Loading

corverroos Jun 1, 2022 •

edited

Loading

corverroos Jun 2, 2022 •

edited

Loading